-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Missing directions/masses in Walk and Tamborra models #231
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these models! A couple things I’ve noticed:
- You’ve included commits from Model registry cleanup #230 in this PR; so we should make sure to merge that one first.
models/Walk_2018/s15.0c_3D_fastrot_dir2_LS220_nux
appears to be an empty file?- We’ve previously (remove duplicate times from Tamborra, Walk models #161, Repeated time entries in Walk_2019 nu_e_bar file #210) had some duplicate time bins in Walk model files. Could you check for duplicate lines in these files and remove them if necessary? (See e.g. this sample code snippet to save some time.)
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Co-authored-by: Jost Migenda <jost.migenda@kcl.ac.uk>
Regarding tests: In Slight problem: Until we’ve tagged v1.3, the model downloader won’t be able to download the files, so we can’t test that. (@sgriswol, am I missing something here?) |
….com/SNEWS2/snewpy into mcolomer/missing_param_tamborra_walk "Modify README to include more information on model parameters"
You're right @JostMigenda, the model downloader attempts to grab models files using a URL that includes a specific version tag (1.2 currently). So until these new model are available under a specific tag, they can't be downloaded. The downloader also needs the correct version to be set in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a quick look and noticed that the new parameter options for Walk_2018
and Walk_2019
were missing from the __new__
methods in a few places. Let me know if I can help get these integrated!
Include new parameters in check - Walk2018 Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Add missing arguments for new parameters - Walk2018 Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Add new parameters in check - Walk2019 Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Add missing new parameters - Walk2019 Co-authored-by: Spencer Griswold <39808718+sgriswol@users.noreply.github.com>
Thanks @sgriswol for your review. Your suggested changes have been added. |
`(1)` is an int in unnecessary brackets; an added comma makes it a tuple
I added some small fixes for the tests. The remaining test failures are due to the download issue mentioned above; that’s unfortunately impossible to fix within the scope of this PR. However, when I manually copy the newly added model files to the cache directory (the |
033ad4a
to
4f73372
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mcolomermolla! I’ve reverted your last commit (updating the model notebooks), because that was already done in another PR (after this one was created), so it created a conflict with the main branch.
Otherwise this looks good; just one more update to the README, then it’s ready to merge
This PR will close issue #78.
For Tambora_2014, I am in touch with Irene, who told me that they did not save the theta and phi used in the three directions of the paper, so for now I can't provide all directions for all masses.